Improve reloading of documents to avoid spurious side-effects #355
Conversation
krassowski
force-pushed
the
more-clever-set
branch
from
57ee1b9 to
5c32dc6
Compare
Automatic application of license header
krassowski
force-pushed
the
more-clever-set
branch
from
5c32dc6 to
9a4a5b5
Compare
jupyter_ydoc/ynotebook.py
Outdated
| # we need to compare against a python cell to avoid | ||
| # an extra transaction on new cells which are not yet | ||
| # integrated into the ydoc document. |
There was a problem hiding this comment.
You could use new_ycell.prelim.get("id") and avoid storing the python cells, or only store python cells and create ycells when needed?
There was a problem hiding this comment.
Good suggestion, I went for only storing python cells as to use prelim we would need to check if a cell is integrated or not and would get more overhead.
jupyter_ydoc/ynotebook.py
Outdated
| index += 1 | ||
|
|
||
| except Exception as e: | ||
| # Fallback to total overwrite, warn to allow debugging |
There was a problem hiding this comment.
What kind of error are you expecting?
There was a problem hiding this comment.
I am not expecting any, unless there is something around concurrency that could go wrong. This is just out of abundance of caution when making a change deep in the stack; do you think we should remove exception handling and the fallback?
There was a problem hiding this comment.
Concurrency like in async? Because this function is not async.
I would rather let the exception propagate if the logic here is wrong, but no strong opinion.
There was a problem hiding this comment.
I would rather let the exception propagate
Done in b97f4de
|
Thank you for the review! I will release this in |
2 participants
This PR addresses the issue of frontends having to re-render widgets (with loss of state!) when any ydoc actor changes the source of the document on top-level. This should be owned by
jupyter-ydocbecause it is the emitter of the signals, and source of truth about primitives (cell/notebook/unicode) structure.For a future PR: repeat on JS side.